Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

support for python 3.5/3.8 and django 2.2 + basic tests #4

Merged
merged 4 commits into from
Jan 6, 2021

Conversation

OmarIthawi
Copy link
Collaborator

@OmarIthawi OmarIthawi commented Dec 29, 2020

I've added support for python 3.5/3.8 and django 2.2 so this XBlock is compatible with the Juniper release.

TODO

  • Tag the current master before merging (for pre-Juniper open edx releases)
  • Enable GitHub Actions
  • Pin requirements in tox.ini
  • Tests passes
  • Clean up # 58, 228-229, 250, 279-288, 307-317, 330-363, 373, 391-396 from tox.ini
  • Installs successfully via $ pip install https://github.com/appsembler/FeedbackXBlock/archive/omar/py3.tar.gz
  • Tag the master after merge (for Juniper)

Stretch Goals

  • Enable BokChoy tests
  • Retain Django 1.11 and Hawthorn compatibility

@OmarIthawi
Copy link
Collaborator Author

@pmitros would you mind giving me some permissions on this repo so I can activate GitHubActions and be able to merge and tag a release?

@pmitros
Copy link
Owner

pmitros commented Dec 30, 2020

@OmarIthawi You should have a request to become a collaborator. I just followed up by email as well.

@OmarIthawi OmarIthawi marked this pull request as ready for review January 4, 2021 12:52
@OmarIthawi OmarIthawi requested a review from pmitros January 4, 2021 12:52
Copy link
Owner

@pmitros pmitros left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me. Please review comment and proceed as you see fit. Thank you for the contribution!

requirements.txt Outdated
XBlock==1.2.1; python_version < '3'
XBlock==1.4.0; python_version >= '3'

xblock-sdk==0.2.2
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Stylistically, I'm not a big fan of locking versions. I'm willing to defer to your lead here, but my preference is >= rather than == for Python 3 versions wherever possible.

Rationale:

  • For development/sandbox/non-deployment settings, I prefer to get the latest version, and fix bugs when and if they occur. Otherwise, you end up with piling debt, and major ports. If you develop with a range of versions, code tends to run up fewer version dependencies.

  • For deployment, I like stable configurations, but I assume versions will come from the install of edX platform. Indeed, locking versions can make a mess if one release of edX wants 1.15 of something, another wants 1.13, and the XBlock is at a loss for which one to lock to.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like this approach. I can actually go with unlocked versions and it won't be bad, except for Python 2.7 compatibility which I like to preserve for a while.

@OmarIthawi OmarIthawi requested a review from pmitros January 5, 2021 09:42
@pmitros
Copy link
Owner

pmitros commented Jan 5, 2021

Looks good to me!

@OmarIthawi OmarIthawi merged commit 712fd1f into pmitros:master Jan 6, 2021
@OmarIthawi OmarIthawi deleted the omar/py3 branch January 6, 2021 05:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants